bugfix(object): Avoid crash with dangling contain module in Object::onDestroy() when Reinforcement Pad is destroyed before Troop Crawler drop by Caball009 · Pull Request #2747 · TheSuperHackers/GeneralsGameCode

Caball009 · 2026-05-25T17:57:18Z

Closes Destroyed Troop Crawler from reinforcement pad may crash the game due to use-after-free bug #2467
Related PR bugfix(contain): Prevent riders from being added to destroyed container object when Reinforcement Pad is destroyed before Troop Crawler drop #2746

This PR provides a retail specific workaround for use-after-free bug that could crash the game under very specific conditions (see issue and related PR).

The actual crash would happen when e.g. OpenContain::m_containList is accessed after it's been destructed. STLPort and MSVC implementations for std::list rely on dynamically allocated memory, so accessing the begin / end iterator after the destruction of the list is not just undefined behavior, it crashes the game. See minimal crash reproduction: https://godbolt.org/z/T9sEb4MsK

TODO:

Verify that resetting the contain pointer is not an alternative (not retail compatible).
Replicate in Generals.

greptile-apps · 2026-05-25T18:00:42Z

Greptile Summary

This PR renames m_xferContainedByID to m_containedByID and expands its role: under RETAIL_COMPATIBLE_CRC, the field is kept continuously in sync with the container's liveness so that onDestroy can safely detect whether m_containedBy has become a dangling pointer, without dereferencing it.

onContainedBy now sets m_containedByID to the container's ID (or INVALID_ID if the container is already destroyed) under RETAIL_COMPATIBLE_CRC, establishing the key invariant the workaround relies on.
onRemovedFrom clears m_containedByID to INVALID_ID, keeping the field in sync.
onDestroy checks m_containedByID == INVALID_ID as a signal that m_containedBy is dangling; if so, removeFromContain is skipped to prevent accessing a freed std::list; in the safe path, findObjectByID confirms the container is alive before calling removeFromContain.
xfer save path is now guarded with !RETAIL_COMPATIBLE_CRC so it does not overwrite the already-maintained m_containedByID with a stale re-derivation (a previously flagged issue, now fixed).

Confidence Score: 5/5

The change is safe to merge; it introduces a well-scoped invariant (INVALID_ID equals dangling pointer) that eliminates the crash-prone removeFromContain call without touching any other code paths.

All four modification sites cooperate correctly to maintain the new invariant. The previously flagged xfer-save re-derivation hole is now properly guarded. The workaround is limited to RETAIL_COMPATIBLE_CRC builds, leaving the non-retail path unchanged.

No files require special attention.

Important Files Changed

Filename	Overview
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp	Core implementation of the workaround; onContainedBy, onRemovedFrom, onDestroy, and xfer all updated correctly; previously flagged xfer-save re-derivation issue is now guarded.
Generals/Code/GameEngine/Source/GameLogic/Object/Object.cpp	Mirrors the GeneralsMD changes; all four modified functions are consistent with the Zero Hour copy.
GeneralsMD/Code/GameEngine/Include/GameLogic/Object.h	Member renamed from m_xferContainedByID to m_containedByID with updated documentation comment reflecting its broadened purpose.
Generals/Code/GameEngine/Include/GameLogic/Object.h	Same header rename as GeneralsMD; no logic changes.

Sequence Diagram

sequenceDiagram
    participant A as Object A (Container)
    participant B as Object B (Contained)
    participant GL as GameLogic
    participant OC as OpenContain (A's module)

    Note over A,B: Normal containment setup
    A->>B: onContainedBy(A)
    B->>B: "m_containedBy = A, m_containedByID = A.getID()"

    Note over A,OC: Container destruction begins (bug scenario)
    GL->>A: destroyObject(A)
    A->>OC: onDelete() — m_containList already freed!
    OC->>B: "onContainedBy(A) where A.isDestroyed()==true"
    B->>B: "m_containedBy = A (dangling!), m_containedByID = INVALID_ID"

    Note over B,GL: Contained object is also destroyed
    GL->>B: destroyObject(B)
    B->>B: onDestroy()
    B->>B: "m_containedBy != null — enter block"
    alt "m_containedByID == INVALID_ID (container already destroyed)"
        B->>B: DEBUG_CRASH / skip removeFromContain (safe)
    else ID valid (normal path)
        B->>GL: findObjectByID(m_containedByID)
        GL-->>B: returns m_containedBy (confirmed alive)
        B->>OC: removeFromContain(B) — safe
    end

_{Reviews (7): Last reviewed commit: "Replicated in Generals." | Re-trigger Greptile}

…sh the game.

Caball009 · 2026-05-26T13:04:05Z

I just did a test in multiplayer with local instances and noticed 6 places where the game would access the container object after destruction. I noticed because the game would crash if the memory is overwritten on deallocation, as is the case with RTS_DEBUG enabled. We cannot fix that, though, because it wouldn't be retail compatible. The issue is probably also more widespread than just these 6 places.

The following 5 could be rewritten like so: T* contain = x->m_containedByID == INVALID_ID ? nullptr : x->get...();

GeneralsGameCode/GeneralsMD/Code/GameEngine/Source/GameLogic/AI/AIStates.cpp

Line 1119 in 92df977

Object *containedBy = obj->getContainedBy();

GeneralsGameCode/GeneralsMD/Code/GameEngine/Source/GameLogic/AI/AIStates.cpp

Line 4940 in 92df977

Object *containedBy = source->getContainedBy();

GeneralsGameCode/GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Weapon.cpp

Line 1903 in 92df977

    
           const ContainModuleInterface *theirContain = source->getContainedBy()->getContain();

GeneralsGameCode/GeneralsMD/Code/GameEngine/Source/GameLogic/Object/WeaponSet.cpp

Line 656 in 92df977

const Object *containedBy = source->getContainedBy();

GeneralsGameCode/GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp

Line 3227 in 92df977

const Object *containedBy = getContainedBy();

An early return if m_containedByID == INVALID_ID in the following function would be needed:

GeneralsGameCode/GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp

Lines 732 to 742 in 92df977

    
           const Object* Object::getEnclosingContainedBy() const 
        
           { 
        
           	for (const Object* child = this, *container = getContainedBy(); container; child = container, container = container->getContainedBy()) 
        
           	{ 
        
           		ContainModuleInterface* containModule = container->getContain(); 
        
           		if (containModule && containModule->isEnclosingContainerFor(child)) 
        
           			return container; 
        
           	} 
        
           	return nullptr; 
        
           }

xezon · 2026-05-27T19:56:48Z

I just did a test in multiplayer with local instances and noticed 6 places where the game would access the container object after destruction. I noticed because the game would crash if the memory is overwritten on deallocation, as is the case with RTS_DEBUG enabled. We cannot fix that, though, because it wouldn't be retail compatible. The issue is probably also more widespread than just these 6 places.

The following 5 could be rewritten like so: T* contain = x->m_containedByID == INVALID_ID ? nullptr : x->get...();

Is this for a follow up or what do we do with this?

Caball009 · 2026-05-31T18:13:36Z

I just did a test in multiplayer with local instances and noticed 6 places where the game would access the container object after destruction. I noticed because the game would crash if the memory is overwritten on deallocation, as is the case with RTS_DEBUG enabled. We cannot fix that, though, because it wouldn't be retail compatible. The issue is probably also more widespread than just these 6 places.
The following 5 could be rewritten like so: T* contain = x->m_containedByID == INVALID_ID ? nullptr : x->get...();

Is this for a follow up or what do we do with this?

Nothing as far as I'm concerned. It's just an explicit acknowledgement that the 'damage' for this bug is so extensive that I don't see how it can be fixed with retail compatibility, but at least it shouldn't crash.

xezon

Needs replication

Resolved merge conflicts in 'Object.cpp'.

Caball009 · 2026-06-01T21:05:55Z

Replicated in Generals.

@BugFix

Upstream: TheSuperHackers/main @ 7c78a5e Ancestor: 20f4254 (previous sync) Divergence: 5 commits upstream / 1813 ours (small sync) PRs brought in: - TheSuperHackers#2710 fix(memory): Fix various memory leaks (2) - TheSuperHackers#2747 bugfix(object): Avoid crash with dangling contain module (Object::onDestroy) - TheSuperHackers#2718 fix/refactor(milesaudiomanager): Prevent multithread crashing + simplify - TheSuperHackers#2577 fix(metaevent): Ignore order in which modifier keys are released - TheSuperHackers#2758 refactor(metaevent): Split MetaEventTranslator::translateGameMessage() Conflict resolution (this commit): - Generals/GeneralsMD/.../MetaEvent.h: replaced m_lastKeyDown/m_lastModState members with upstream's KeyDownInfo m_keyDownInfos[KEY_COUNT] struct to match the refactored translator (auto-merge had unioned both). - Generals/GeneralsMD/.../MetaEvent.cpp: resolved UU by taking upstream's onKeyEvent() dispatch + dropped now-dead m_lastKeyDown/m_lastModState member initializers in the constructor. - Core/GameEngine/Include/Common/GameAudio.h: kept upstream's signature change (nextMusicTrack/prevMusicTrack now return AsciiString; getMusicTrackName removed from base). Updated our OpenAL backend (Core/GameEngineDevice/{Include,Source}/OpenALAudioDevice/) to match. - Core/Libraries/Include/Lib/BaseType.h: added missing BitsAreSet macro that upstream's MetaEvent code depends on. - AGENTS.md: cleared pre-existing merge markers (not from this sync; left from commit 996eedd). - GeneralsMD/.../Source/OpenALAudioManager.cpp stub: signature parity. Preserved (no merge change): - GeneralsX cross-platform stack: SDL3, DXVK, OpenAL, FFmpeg - All .github/ CI/CD infrastructure intact - GeneralsX-specific patches and GeneralsX @BugFix annotations Verified: - macos-vulkan configure + build (GeneralsX + GeneralsXZH) - Runtime smoke for both products (clean init, no crash) - No merge markers in tree (excluding build/ artifacts) - Cross-platform audit: no platform-specific code leaked into shared headers; SDL3/DXVK/OpenAL/FFmpeg layers untouched by upstream Deferred to CI: - linux64-deploy configure + build (validated locally but heavy docker image build; CI covers) See docs/WORKDIR/planning/PLAN-2026-06-04_THESUPERHACKERS_SYNC.md for the full conflict resolution report and risk analysis. Refs: TheSuperHackers#2710, TheSuperHackers#2747, TheSuperHackers#2718, TheSuperHackers#2577, TheSuperHackers#2758

@BugFix

Upstream: TheSuperHackers/main @ 7c78a5e Ancestor: 20f4254 (previous sync) Divergence: 5 commits upstream / 1813 ours (small sync) PRs brought in: - TheSuperHackers#2710 fix(memory): Fix various memory leaks (2) - TheSuperHackers#2747 bugfix(object): Avoid crash with dangling contain module (Object::onDestroy) - TheSuperHackers#2718 fix/refactor(milesaudiomanager): Prevent multithread crashing + simplify - TheSuperHackers#2577 fix(metaevent): Ignore order in which modifier keys are released - TheSuperHackers#2758 refactor(metaevent): Split MetaEventTranslator::translateGameMessage() Conflict resolution (this commit): - Generals/GeneralsMD/.../MetaEvent.h: replaced m_lastKeyDown/m_lastModState members with upstream's KeyDownInfo m_keyDownInfos[KEY_COUNT] struct to match the refactored translator (auto-merge had unioned both). - Generals/GeneralsMD/.../MetaEvent.cpp: resolved UU by taking upstream's onKeyEvent() dispatch + dropped now-dead m_lastKeyDown/m_lastModState member initializers in the constructor. - Core/GameEngine/Include/Common/GameAudio.h: kept upstream's signature change (nextMusicTrack/prevMusicTrack now return AsciiString; getMusicTrackName removed from base). Updated our OpenAL backend (Core/GameEngineDevice/{Include,Source}/OpenALAudioDevice/) to match. - Core/Libraries/Include/Lib/BaseType.h: added missing BitsAreSet macro that upstream's MetaEvent code depends on. - AGENTS.md: cleared pre-existing merge markers (not from this sync; left from commit 996eedd). - GeneralsMD/.../Source/OpenALAudioManager.cpp stub: signature parity. Preserved (no merge change): - GeneralsX cross-platform stack: SDL3, DXVK, OpenAL, FFmpeg - All .github/ CI/CD infrastructure intact - GeneralsX-specific patches and GeneralsX @BugFix annotations Verified: - macos-vulkan configure + build (GeneralsX + GeneralsXZH) - Runtime smoke for both products (clean init, no crash) - No merge markers in tree (excluding build/ artifacts) - Cross-platform audit: no platform-specific code leaked into shared headers; SDL3/DXVK/OpenAL/FFmpeg layers untouched by upstream Deferred to CI: - linux64-deploy configure + build (validated locally but heavy docker image build; CI covers) See docs/WORKDIR/planning/PLAN-2026-06-04_THESUPERHACKERS_SYNC.md for the full conflict resolution report and risk analysis. Refs: TheSuperHackers#2710, TheSuperHackers#2747, TheSuperHackers#2718, TheSuperHackers#2577, TheSuperHackers#2758

Caball009 added Bug Something is not working right, typically is user facing Major Severity: Minor < Major < Critical < Blocker Gen Relates to Generals ZH Relates to Zero Hour Crash This is a crash, very bad labels May 25, 2026

Caball009 mentioned this pull request May 25, 2026

bugfix(contain): Prevent riders from being added to destroyed container object when Reinforcement Pad is destroyed before Troop Crawler drop #2746

Merged

1 task

greptile-apps Bot reviewed May 25, 2026

View reviewed changes

Comment thread GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp Outdated

xezon reviewed May 25, 2026

View reviewed changes

Comment thread GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp

Caball009 force-pushed the fix_crash_removeFromContain branch 2 times, most recently from 3c40642 to 69cfc9b Compare May 25, 2026 21:32

bugfix(contain): Add workaround for use-after-free bug that could cra…

69cfc9b

…sh the game.

xezon reviewed May 26, 2026

View reviewed changes

Comment thread GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp

Comment thread GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp Outdated

Comment thread GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp

Caball009 force-pushed the fix_crash_removeFromContain branch from da36906 to 1af8f31 Compare May 26, 2026 09:54

Addressed feedback.

1af8f31

greptile-apps Bot reviewed May 26, 2026

View reviewed changes

Comment thread GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp

Caball009 added 2 commits May 26, 2026 12:28

Fixed xfer logic.

3ae7e44

Moved debug assertion.

6b48539

xezon reviewed May 26, 2026

View reviewed changes

Addressed feedback.

d49db75

xezon approved these changes Jun 1, 2026

View reviewed changes

Caball009 force-pushed the fix_crash_removeFromContain branch 2 times, most recently from 7635f14 to 756de02 Compare June 1, 2026 20:45

Replicated in Generals.

756de02

Resolved merge conflicts in 'Object.cpp'.

xezon changed the title ~~bugfix(contain): Add workaround for use-after-free bug that may crash the game~~ bugfix(contain): Avoid crash with contain module in Object::onDestroy() when Reinforcement Pad is destroyed before Troop Crawler drop Jun 3, 2026

xezon merged commit df2224b into TheSuperHackers:main Jun 3, 2026
17 checks passed

Caball009 deleted the fix_crash_removeFromContain branch June 4, 2026 14:53

This was referenced Jun 5, 2026

sync: TheSuperHackers upstream (5 commits, 06-04-2026) fbraz3/GeneralsX#154

Closed

[sync] TheSuperHackers upstream sync 06-05-2026 fbraz3/GeneralsX#155

Merged

Conversation

Caball009 commented May 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO:

Uh oh!

greptile-apps Bot commented May 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Sequence Diagram

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Caball009 commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xezon commented May 27, 2026

Uh oh!

Caball009 commented May 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

Caball009 commented Jun 1, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Caball009 commented May 25, 2026 •

edited

Loading

greptile-apps Bot commented May 25, 2026 •

edited

Loading

Caball009 commented May 26, 2026 •

edited

Loading

Caball009 commented May 31, 2026 •

edited

Loading